Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add number of container using image #15741

Merged
merged 1 commit into from
Aug 25, 2017
Merged

Add number of container using image #15741

merged 1 commit into from
Aug 25, 2017

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Aug 6, 2017 •
edited
Loading

This is to support adding Number of Containers column to the ContainerImage list view.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1474094
Screenshot of report :
29004803-a7b47b0c-7ad6-11e7-9061-c2c77b09bc78

cc: @enoodle @Loicavenel
@simon3z

@nimrodshn nimrodshn changed the title add number of container using image Add number of container using image Aug 6, 2017
@nimrodshn nimrodshn mentioned this pull request Aug 6, 2017
Merged
@nimrodshn
Copy link
Contributor Author

nimrodshn commented Aug 6, 2017 •
edited
Loading

@miq-bot add_label providers/containers, bug

@miq-bot
Copy link
Member

miq-bot commented Aug 6, 2017

@nimrodshn Cannot apply the following label because they are not recognized: containers/providers

@miq-bot miq-bot added the bug label Aug 6, 2017
@nimrodshn
Copy link
Contributor Author

@miq-bot add_label providers/containers

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from 4b4b744 to 702b048 Compare August 6, 2017 15:49
enoodle
enoodle approved these changes Aug 7, 2017
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@enoodle
Copy link

enoodle commented Aug 7, 2017

@nimrodshn I think you should restart the testing. Maybe even add a simple test to make sure that this functionality doesn't change by accident in the future.

@chessbyte chessbyte added enhancement and removed bug labels Aug 7, 2017
@nimrodshn nimrodshn closed this Aug 8, 2017
@nimrodshn nimrodshn reopened this Aug 8, 2017
@nimrodshn nimrodshn mentioned this pull request Aug 8, 2017
Closed
@nimrodshn
Copy link
Contributor Author

@enoodle added tests 👍

moolitayer
moolitayer reviewed Aug 8, 2017
View reviewed changes
app/models/container_image.rb Outdated
@@ -31,9 +31,14 @@ class ContainerImage < ApplicationRecord

acts_as_miq_taggable
virtual_column :display_registry, :type => :string
virtual_column :number_of_containers, :type => :integer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: container_count?

@zeari @cben what does this actually mean? the number of running containers?
Does that include archived containers? (we probably don't want those)

cben
cben reviewed Aug 8, 2017
View reviewed changes
app/models/container_image.rb Outdated

after_create :raise_creation_event

def number_of_containers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a simple count you can use virtual_total, no need for virtual_column with custom method. Naming: there is a convention of total_containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cben but for a report I think I need it as a column, keep me honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind this property is used in a report.

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from 702b048 to c2298f9 Compare August 8, 2017 12:38
@nimrodshn
Copy link
Contributor Author

@cben changed to virtual_total.
@moolitayer renamed and added tests.

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from c2298f9 to df8d442 Compare August 8, 2017 13:08
moolitayer
moolitayer reviewed Aug 8, 2017
View reviewed changes
app/models/container_image.rb Outdated
@@ -31,6 +31,7 @@ class ContainerImage < ApplicationRecord

acts_as_miq_taggable
virtual_column :display_registry, :type => :string
virtual_total :containers_count, :containers
Copy link

@moolitayer moolitayer Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beni indeed I see there is a stronger convention toward total_containers @nimrodshn can you please rename (again ¯_(ツ)_/¯)

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from df8d442 to b1bd6d0 Compare August 8, 2017 13:37
@nimrodshn
Copy link
Contributor Author

@moolitayer fixed 👍

moolitayer
moolitayer reviewed Aug 8, 2017
View reviewed changes
spec/models/container_image/container_image_spec.rb Outdated
expect(FactoryGirl.create(
:container_image,
:containers => [FactoryGirl.create(:container, :name => "container_a", :container_group => group),
FactoryGirl.create(:container, :name => "container_b", :container_group => group)]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

:containers => FactoryGirl.create_list(:container, :container_group => group)

You will probably need to add a sequence definition in the container factory.

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch 2 times, most recently from 82b1d33 to d190e52 Compare August 9, 2017 10:19
enoodle
enoodle reviewed Aug 9, 2017
View reviewed changes
spec/models/container_image/container_image_spec.rb Outdated
@@ -0,0 +1,13 @@
describe ContainerImage do
it "containers count" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it "counts containers" do ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from d190e52 to 58dcedb Compare August 9, 2017 10:21
@nimrodshn
Copy link
Contributor Author

@enoodle , @moolitayer fixed 👍
PTAL

moolitayer
moolitayer reviewed Aug 9, 2017
View reviewed changes
1 Outdated
@@ -0,0 +1,21 @@
pick 82b1d33 add number of container using image
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file please and I'm good

@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from 58dcedb to b04db6b Compare August 9, 2017 10:27
@nimrodshn
Copy link
Contributor Author

@moolitayer fixed

moolitayer
moolitayer approved these changes Aug 9, 2017
View reviewed changes
Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nimrodshn
LGTM 👍

@nimrodshn
Copy link
Contributor Author

@gtanzillo @lpichler I use this virtual_total in a report (ManageIQ/manageiq-ui-classic#1840). Please review 😇

@cben
Copy link
Contributor

cben commented Aug 9, 2017

please rebase (the "test saver strategies" commit should disappear) and squash "removed random file" commit.

@nimrodshn
add number of container using image
140d1aa 
some refactoring

refactored containers count

minor refactoring

minor refactoring

refactored tests

refactored tests

refactored tests

added file by mistake

removed random file
@nimrodshn nimrodshn force-pushed the add_features_container_image_view branch from 61b6d04 to 140d1aa Compare August 10, 2017 13:12
@nimrodshn
Copy link
Contributor Author

@cben Thanks! fixed 😃

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2017

Checked commit nimrodshn@140d1aa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

zeari
zeari approved these changes Aug 10, 2017
View reviewed changes
Copy link

@zeari zeari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot of a report please

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Aug 13, 2017 •
edited
Loading

@zeari screenshot at the top ⬆️

@simon3z
Copy link
Contributor

simon3z commented Aug 25, 2017

@miq-bot assign agrare
LGTM :+1 cc @agrare

@miq-bot miq-bot assigned agrare and unassigned simon3z Aug 25, 2017
@agrare
Copy link
Member

agrare commented Aug 25, 2017

❤️ awesome @nimrodshn

@agrare agrare merged commit f9f8fa4 into ManageIQ:master Aug 25, 2017
@agrare agrare added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enoodle enoodle enoodle approved these changes

@lpichler lpichler lpichler approved these changes

@zeari zeari zeari approved these changes

@moolitayer moolitayer moolitayer approved these changes

@cben cben cben approved these changes

@simon3z simon3z simon3z approved these changes

Assignees

@agrare agrare

Labels
enhancement providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@nimrodshn @miq-bot @enoodle @cben @simon3z @agrare @moolitayer @zeari @lpichler @chessbyte